Surface Retry-After so rate-limited visitors know when to retry#49
Conversation
|
While reading the form gateway I noticed the It's intentionally defensive — a missing/odd header just yields no hint, so current behavior is preserved. Added a worker test asserting the header is exposed; |
mberman84
left a comment
mberman84
left a comment
There was a problem hiding this comment.
The Worker correctly exposes Retry-After through CORS, and the integrated Worker suite passes. Two client-side fixes remain:
Math.round(seconds / 60)can understate the server's minimum delay by up to 29 seconds (for example, 3,569 seconds is shown as 59 minutes). Please useMath.ceil(seconds / 60)so the guidance never sends the visitor back before the limit expires.- This changes
site/script.jswithout changing the current versioned script URL. Please bump the shared script asset version everywhere it is emitted/validated and regenerate affected pages so cached visitors receive the new behavior.
|
Both addressed: switched the seconds→minutes (and seconds) conversion to |
|
@mberman84 ready for another look — |
mberman84
left a comment
mberman84
left a comment
There was a problem hiding this comment.
Correction to my immediately preceding review, whose inline code formatting was stripped by the shell:
Both previously requested fixes are addressed: the retry hint now rounds up, and the script asset is cache-busted. The exact head passes the site checks/builders and the Worker suite (14/14); the CORS exposure and client behavior look correct.
This branch now needs a rebase onto current main before it can be approved. Since this PR branched, #61/#62 moved loop detail pages to the database-backed proxy and removed scripts/build-loop-pages.mjs plus site/loops/**. GitHub reports this head as conflicting, and resolving by restoring those stale files would regress the new architecture.
Please rebase onto the latest main, retain the focused site/script.js, Worker implementation, and Worker test changes, update only surviving script references/check assertions to a fresh version, and leave the removed generator/static loop pages deleted. Then rerun node scripts/check.mjs and npm --prefix worker run check.
mberman84
force-pushed
the
fix/expose-retry-after
branch
from
035f7f1 to
5232d89
Compare
mberman84
left a comment
mberman84
left a comment
There was a problem hiding this comment.
Rebased onto the database-backed main architecture. The focused retry behavior and Worker CORS test are preserved, stale static-page artifacts are gone, and the full current CI-equivalent suite passes locally (41/41 Worker tests).
2 participants
Summary
The form gateway already computes and returns a
Retry-Afterheader on429responses (both the verification rate limiter and the per-IP hourly/daily limiter), but it never reached the visitor:Retry-Afteris not a CORS-safelisted response header, so a cross-originfetch()from the site can't read it unless it's listed inAccess-Control-Expose-Headers— which it wasn't.postProtectedForm()surfaced onlyresponseBody.error, so a rate-limited visitor saw "Try again later" with no sense of how long.Fix
worker/src/index.js): addAccess-Control-Expose-Headers: Retry-Afterto the CORS headers so the browser can read it.site/script.js): on a429, readRetry-Afterand append a concrete hint — e.g. "Try again in about 1 minute." — to the existing status message.worker/test/index.test.js): assert the header is exposed on rate-limited responses.The hint formatting is defensive: a missing or non-numeric header simply yields no hint, preserving today's behavior.
Verification
npm --prefix worker run check→ 14/14 tests pass.node scripts/check.mjs→Loop Library checks passed.node --check site/script.js